Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tado service set temperature offset #45014

Merged
merged 12 commits into from Jan 26, 2021
Merged

Add tado service set temperature offset #45014

merged 12 commits into from Jan 26, 2021

Conversation

north3221
Copy link
Contributor

@north3221 north3221 commented Jan 10, 2021

Proposed change

Tado devices have a temperature offset which Home Assistant currently cannot change, this PR provides the ability to change the offset value with a new service. This will let people adjust the offset with automations, using other devices to decide the offset

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Contributor

Hi @north3221,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link

Hey there @michaelarnauts, @bdraco, mind taking a look at this pull request as its been labeled with an integration (tado) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@MartinHjelmare MartinHjelmare changed the title Add ability to set temperature offset and corresponding service Add tado ability to set temperature offset and corresponding service Jan 10, 2021
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Code owner should approve before merge.

@north3221
Copy link
Contributor Author

I've added some code to get the current offset and add it as an attribute on the zone. I'm not convinced its the best way to do it, as maybe making another change to PyTado to add it to the TadoZone object might be better, but the issue there is the zone doesn't have knowledge of the device to get the offset...

The way I have done it works. I have put it as an output to in 'device_state_attributes' as that seems like the logical place? Unless I am misunderstanding the docs (I've very new to this and my first HA PR), you can't create your own properties so need to use standard climate ones?

Alternatively could set up on individual devices, but they have been set as binary_sensors, doesnt feel right. I think maybe adding devices to zones as that's how Tado see them may be better, but that's a fundamental change to how this is set up. Anyhow it works :-)

I'll push the commit and can undo that commit if that part isn't accepted and do a separate PR for it a different way depending on feedback.

@north3221
Copy link
Contributor Author

Ahh should of added some constants to .const for that... I'll let someone review and see if there is anything else to fix and do at same time

@north3221
Copy link
Contributor Author

@MartinHjelmare fab thanks for all the help. Think that should now do it.

@north3221
Copy link
Contributor Author

north3221 commented Jan 13, 2021

Ignore... I worked it out (I think) - feedback very welcome on the commit to add it


Hey could anyone help explain how I make the service accept a template? I tried and failed.... OR point me at docs that explain it...

It didn't seem to be there is the developer docs. It seems if I change it to allow string or template it passes it as a full string of the template, not the value of the template or errors with 'Object of type Template is not JSON serializable'.

Assume not as straightforward as I thought? So do you allow string then write code in your function to test for template and execute template to get he value then validate value?

I kinda expected I could say accept float or template that results in float?

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change things after the PR has been approved unless asked for.

homeassistant/components/tado/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MartinHjelmare
Copy link
Member

Please document the new service and add a link to the docs PR in this PR description:
https://www.home-assistant.io/integrations/tado/#services

We can merge when that is done and code owner approves.

@north3221
Copy link
Contributor Author

Please document the new service and add a link to the docs PR in this PR description:
https://www.home-assistant.io/integrations/tado/#services

We can merge when that is done and code owner approves.

Sorry been a mental week, finally got round to doing this and added link. NB the pr has a label of 'current' but I ticked the 'documenting new feature I am adding', which I thought would make it 'next' branch. Not sure if I did something wrong?

@north3221
Copy link
Contributor Author

north3221 commented Jan 25, 2021

Raised a new doc pr as couldn't change to next in the gui, updated link in this one to correct docs pr

hopefully now should all be good to go

@MartinHjelmare
Copy link
Member

@bdraco and @michaelarnauts ok to merge?

@michaelarnauts
Copy link
Contributor

This looks fine!

@MartinHjelmare MartinHjelmare changed the title Add tado ability to set temperature offset and corresponding service Add tado service set temperature offset Jan 26, 2021
@MartinHjelmare MartinHjelmare merged commit f2a8ccd into home-assistant:dev Jan 26, 2021
@north3221 north3221 deleted the add-temp-offset branch January 26, 2021 11:13
@north3221
Copy link
Contributor Author

Just noticed a duplicate feature request for this: https://community.home-assistant.io/t/tado-integration-put-offset/175964/3

Not sure if the feature requests threads get closed after implementation? Is so just adding here so it can be closed

@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants